-
Notifications
You must be signed in to change notification settings - Fork 0
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: add blob/get #126
feat: add blob/get #126
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
needs origin out though
Co-authored-by: Vasco Santos <santos.vasco10@gmail.com>
w3-blob.md
Outdated
} | ||
|
||
type GetBlobOk = { | ||
blob: blob |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I saw in the implementation you add the cause
here. And I actually think that is nice, so also should be in the spec response
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
makes perfect sense! adding it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly looks good, just some minor fixes needed.
Co-authored-by: Alan Shaw <alan.shaw@protocol.ai>
Co-authored-by: Alan Shaw <alan.shaw@protocol.ai>
Co-authored-by: Alan Shaw <alan.shaw@protocol.ai>
Co-authored-by: Alan Shaw <alan.shaw@protocol.ai>
Co-authored-by: Alan Shaw <alan.shaw@protocol.ai>
Co-authored-by: Alan Shaw <alan.shaw@protocol.ai>
Co-authored-by: Alan Shaw <alan.shaw@protocol.ai>
This might be controversial, but I wonder if we could simply return return pointer to the receipt and let client fetch all the details. The rational here is that adding a blob is a long running task and has state that moves from initiated to in progress to complete described by individual tasks. I think it would make most sense to do that. That is not to say we should block shipping blob/get on this, but perhaps it would be better to not spec out in way that requires state checks on the server. Please note that I'm not advocating for the above solution, it's really more of a question than prescription. |
In the implementation PR I have proposed adding version suffix to the capability storacha/w3up#1484 (review) I think it would make sense to add this changes to the spec with version suffix also maybe with some note that likely we'll deprecate it in the future in favor of suffixes-less version that just returns receipt id. |
Adds support for `blob/get` as defined in storacha/specs#126 --------- Co-authored-by: Vasco Santos <santos.vasco10@gmail.com> Co-authored-by: Irakli Gozalishvili <contact@gozala.io>
w3-blob.md
Outdated
message: string | ||
} | ||
|
||
type Blob = { /* ??? */ } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you need to fill this out - it's multihash and size right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good point. completely missed it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@alanshaw addressed
Adds the
blob/get
capability spec which is analogous to #82